feat(metadata-editor): wire new MetadataTaxonomyPicker into redesigned sidebar#4683
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds taxonomy response normalization and a taxonomy items service, wires it through the metadata sidebar and editor behind a feature flag, updates package versions, and adds coverage for fetching, drill-down, search, pagination, and component integration. ChangesTaxonomy Picker Service
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MetadataSidebarRedesign
participant MetadataInstanceEditor
participant MetadataInstanceForm
participant createTaxonomyItemsService
participant getMetadataOptions
MetadataSidebarRedesign->>MetadataInstanceEditor: pass feature flag and service creator
MetadataInstanceEditor->>MetadataInstanceForm: pass taxonomy-picker props
MetadataInstanceForm->>createTaxonomyItemsService: request taxonomy nodes
createTaxonomyItemsService->>getMetadataOptions: fetch scoped options
getMetadataOptions-->>createTaxonomyItemsService: return options and marker
createTaxonomyItemsService-->>MetadataInstanceForm: return normalized nodes
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.ts (1)
623-630: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the positional
levelarg here to fully cover the "safe default" contract.This test verifies
optionshas nolevel, but does not assert the positionallevelargument (mock.calls[0][4]). GivengetNodesalways passes a computedchildLevelpositionally (seemetadataTaxonomyFetcher.ts), the unrestricted-browse guarantee isn't actually exercised. Adding an assertion onmock.calls[0][4]would catch the divergence fromsearchNodes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.ts` around lines 623 - 630, The safe-default test only checks the options object and misses the positional level argument passed by createTaxonomyItemsService/getNodes. Update the test in metadataTaxonomyFetcher.test.ts to also assert the fifth mock call argument (mock.calls[0][4]) for the expected unrestricted-browse childLevel behavior, so it covers the same contract that getNodes and searchNodes rely on.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.ts`:
- Around line 623-630: The safe-default test only checks the options object and
misses the positional level argument passed by
createTaxonomyItemsService/getNodes. Update the test in
metadataTaxonomyFetcher.test.ts to also assert the fifth mock call argument
(mock.calls[0][4]) for the expected unrestricted-browse childLevel behavior, so
it covers the same contract that getNodes and searchNodes rely on.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f2f1982-179b-40c7-b4a6-5912790a1336
📒 Files selected for processing (5)
src/elements/content-sidebar/MetadataInstanceEditor.tsxsrc/elements/content-sidebar/MetadataSidebarRedesign.tsxsrc/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.tssrc/elements/content-sidebar/fetchers/metadataTaxonomyFetcher.tssrc/elements/content-sidebar/stories/tests/MetadataSidebarRedesign-visual.stories.tsx
AnnaStepchenko
force-pushed
the
taxonomy-picker-update-for-preview
branch
from
bdfd260 to
33c2ed1
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| createTaxonomyItemsService={ | ||
| isMetadataTaxonomyPickerEnabled ? taxonomyItemsServiceCreator : undefined | ||
| } |
There was a problem hiding this comment.
nit: this list is alphabetical
There was a problem hiding this comment.
Moved the prop from between taxonomyOptionsFetcher and template up to right after areAiSuggestionsAvailable 👍 86164ef
There was a problem hiding this comment.
we should be cautious of how many VRTs we're adding. how many of these are necessary as VRTs vs RTL test?
i.e. does the shared feature already cover the UI states? if so, then let's convert the tests to RTL to save costs of VRTs
There was a problem hiding this comment.
Good point, the picker's actual UI states are already covered by VRTs in the shared feature. Added a new focused RTL suite MetadataSidebarRedesign-taxonomyPicker.test.tsx that verifies the integration & removed VRTs 86164ef
| if (typeof option.has_children === 'boolean') { | ||
| return option.has_children; | ||
| } | ||
| if (typeof option.hasChildren === 'boolean') { | ||
| return option.hasChildren; | ||
| } |
There was a problem hiding this comment.
is this type correct? I don't see these properties on MetadataOptionEntry. why does it have both?
There was a problem hiding this comment.
Replaced it with a new local type TaxonomyEntry that spells out every field we actually read, including both the snake_case and camelCase variants, with a comment explaining why both exist 86164ef
| }; | ||
|
|
||
| // Prefers backend `has_children`; falls back to depth. `forceLeaf` disables drill-down. | ||
| const getHasChildren = (option: MetadataOptionEntry, maxLevel?: number, forceLeaf?: boolean): boolean => { |
There was a problem hiding this comment.
what does forceLeaf mean? if it disables "drill-down" then isn't it forcing the root instead of the leaf?
There was a problem hiding this comment.
A "leaf" here means "no children / no drill-down chevron", which is what single-level picker mode requires for every row. I understand why it can look weird, renamed it to isSingleLevelMode and updated its docstring
| for (const entry of entries) { | ||
| if (entry?.id != null && entry.level != null) { | ||
| nodeLevelById.set(String(entry.id), Number(entry.level)); | ||
| } | ||
| if (Array.isArray(entry?.ancestors)) { | ||
| for (const ancestor of entry.ancestors) { | ||
| if (ancestor && typeof ancestor === 'object' && ancestor.id != null && ancestor.level != null) { | ||
| nodeLevelById.set(String(ancestor.id), Number(ancestor.level)); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
the new changes in this file has a lot of loose equality checks with != null. do we know why are all the properties from fieldConfig optional?
are we able to add defaults to fieldConfig to simplify the logic instead of having many conditionals / ternaries / null coalescing operators?
something like:
const { levels = [], selectableLevels = [], ... } = resolveField?.(templateKey, fieldKey);
const maxLevel: number = ... // instead of `getMaxLevel` returning number | undefined, can it just return number?
There was a problem hiding this comment.
Good point, simplified the factory setup using destructuring defaults 86164ef
| has_children?: boolean; | ||
| hasChildren?: boolean; | ||
| selectable?: boolean; | ||
| ancestors?: Array<TaxonomyEntryAncestor | string | null | undefined>; |
There was a problem hiding this comment.
is this type correct? an ancestor can be all of these?
There was a problem hiding this comment.
why would it be an array of null or undefined?
| // API may return level as number ('1') or string ('"1"'); callers coerce. | ||
| level: number | string; |
There was a problem hiding this comment.
is this true? the API could return either type?
| // Tracks levels of returned nodes so drill-down can request `parentLevel + 1`. | ||
| const nodeLevelById = new Map<string, number>(); | ||
|
|
||
| const rememberLevels = (entries: TaxonomyEntry[]): void => { |
There was a problem hiding this comment.
what does rememberLevels mean?
| if (entry?.id != null && entry.level != null) { | ||
| nodeLevelById.set(String(entry.id), Number(entry.level)); | ||
| } | ||
| for (const ancestor of entry?.ancestors ?? []) { | ||
| if (ancestor && typeof ancestor === 'object' && ancestor.id != null && ancestor.level != null) { | ||
| nodeLevelById.set(String(ancestor.id), Number(ancestor.level)); | ||
| } | ||
| } |
There was a problem hiding this comment.
I still see a lot of loose equality throughout this file. if we're looping through entries, when would entry?.id != null ever be false?
similar for entry.level != null? based on the TaxonomyEntry type, these properties always exist
tjuanitas
left a comment
tjuanitas
left a comment
There was a problem hiding this comment.
comments but no major concerns
Merge Queue Status
This pull request spent 16 seconds in the queue, including 3 seconds running CI. Required conditions to merge
|
3 participants
Summary
Wires the new
MetadataTaxonomyPicker(from@box/metadata-taxonomy-picker, consumed via@box/metadata-editor) into the redesigned metadata sidebar behind a newmetadata.taxonomyPicker.enabledfeature flag. When the flag is off, the sidebar keeps rendering the legacyMultilevelTaxonomyField— no behavior change for existing consumers.What's in this PR
Fetcher —
src/elements/content-sidebar/fetchers/metadataTaxonomyFetcher.tscreateTaxonomyItemsServicefactory that returns per-fieldTaxonomyItemsServices (browse + search) backed by the existing/metadata_templates/.../optionsendpoint.optionsRules.selectableLevels:selectableLevelshas exactly one entry): loads that level directly (level=N, no ancestor) and marks every returned node as a leaf so no drill-down chevron renders.selectableLevels.length > 1): starts at level 1 and drills down on demand, tracking each returned node's level so it can requestparentLevel + 1withancestor=<parentId>.Sidebar wiring
MetadataSidebarRedesign.tsx— reads the newmetadata.taxonomyPicker.enabledflag, resolves the editing template's field config (levels+optionsRules.selectableLevels), memoizes acreateTaxonomyItemsServiceinstance, and threads it intoMetadataInstanceEditor.MetadataInstanceEditor.tsx— accepts the newisMetadataTaxonomyPickerEnabledandcreateTaxonomyItemsServiceprops and forwards them toMetadataInstanceForm(asisTaxonomyPickerEnabledandcreateTaxonomyItemsService).Summary by CodeRabbit